Codecov Report

Patch coverage: 100.000% and project coverage change: +0.054 🎉

Comparison is base (5ee36fd) 88.607% compared to head (064f63b) 88.661%.

@@              Coverage Diff              @@
##              main     #2697       +/-   ##
=============================================
+ Coverage   88.607%   88.661%   +0.054%     
=============================================
  Files           87        87               
  Lines         6820      6844       +24     
  Branches      1171      1178        +7     
=============================================
+ Hits          6043      6068       +25     
  Misses         535       535               
+ Partials       242       241        -1     

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Any update on this?

Hi there, looks good. Can you please add a test to prevent a regression?

This is my first time contributing to Sanic, could you please show me how to add test?

This is my first time contributing to Sanic, could you please show me how to add test?

Sure. Sanic uses pytest for testing (you can find guides for that online) and you can run the tests on your own machine by

# Replace py311 with your Python version (here 3.11)
tox -e py311 -- tests/test_response.py

Or alternatively directly with pytest, without tox. If you leave out the double hyphen and test filename, it will run all tests which will take some time. Before changing anything, see that the tests pass OK.

Since your patch is in sanic/request/, the appropriate test file is probably that mentioned in the example above. It might already have a test function that tests the validate_file function. You might be able to modify an existing test function, or simply add your own function named test_validate_file_datetime, where you call the function in a way that without your patch would raise an exception.

When doing bugfixes and tests for them, it is a good practice to then try running your test without your patch, to see that it actually fails, and then apply the patch and see that it passes.

Please ask if you have any further questions, we'll be happy to help getting started with that!

@stricaud Hey, how are you doing? Let me know if you need help with the test.

I am trying to write the test, however it has been many days and I don't see how to reproduce this problem.
I do not have much time. I will see if I can live with a non patched version and see the problem coming again to be able to write the test.

@stricaud I think I see what you are after. Do you mind if I make some pushes to your branch?

I don't mind! Very welcome

I'd like to make sure that the fix proposed is the right one: what causes tz-aware and unaware timestamps being mixed here, can we make both either tz-aware or not aware? If not, is there a chance that one of them would be in local tz, and produce wrong timestamp value?

If not, is there a chance that one of them would be in local tz, and produce wrong timestamp value?

Yup. I am not sure how best to avoid this without going too deep and over-engineering this. My vote is as the implementation I pushed: handle when they are both aware and both naive. If there is a mismatch, add the missing tz as UTC and throw up a warning.